Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#183: removed isInstalledVersion() #184

Conversation

MattesMrzik
Copy link
Contributor

Fixes #183. isInstalledVersion() was only used once and caused the duplicate msg if the tool was already installed.

this method was only used once and caused the duplicate msg if the tool was already installed.
@coveralls
Copy link
Collaborator

coveralls commented Jan 16, 2024

Pull Request Test Coverage Report for Build 7612283729

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.04%) to 54.912%

Totals Coverage Status
Change from base Build 7574035726: -0.04%
Covered Lines: 3294
Relevant Lines: 5772

💛 - Coveralls

Copy link
Member

@hohwille hohwille left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed we have this log message duplicated:

So thanks @MattesMrzik for noticing and taking action to improve. 👍
However, using the silent flag to determine the log-level is an intended behavior we have taken from devonfw-ide.
The idea is as following:

$ devon mvn setup
Version 3.9.6 of tool mvn is already installed

This is the intended default behavior. But also consider this:

$ devon --debug mvn setup
Version 17.0.8 of tool java is already installed
Version 3.9.6 of tool mvn is already installed

By default the user would not expect to get this info about java and that would come every time we run mvn if we log it statically on info or it would come never by default if we log statically on debug so devon java setup would give no feedback at all by default what would be confusing:

$ devon java setup
$

Your change is fine, but you should also add the dynamic logging part from isInstalledVersion method to ToolCommandlet as a protected method that is used by Local/GlobalToolCommandlet. I missed this on the review of #113

UPDATE: I just see that in GlobalToolCommandlet we do not have a version so you could still do what I suggested but allow GlobalToolCommandlet to pass null as version parameter and log accordingly.

@salimbouch salimbouch assigned MattesMrzik and unassigned salimbouch Jan 17, 2024
@salimbouch salimbouch requested review from salimbouch and removed request for salimbouch January 17, 2024 11:40
Copy link
Contributor

@salimbouch salimbouch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just like @hohwille said we need this dynamic logging otherwise we don't get any output when not in debugger mode. If I'm not mistaken you can move isInstalledVersion to ToolCommandlet and use it both in Global and Local, maybe call it isInstalled since Global Tools don't have any version.

Copy link
Contributor

@salimbouch salimbouch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me 👍

@MattesMrzik MattesMrzik assigned hohwille and unassigned salimbouch Jan 18, 2024
Copy link
Member

@hohwille hohwille left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, PR does not avoid the redundancies but functionally now correct and ready for merge.
I commented on #52 because of the pointless line-wrapping changes, but this can now be merged. Thanks again 👍

@hohwille hohwille merged commit 3dfa78f into devonfw:main Jan 22, 2024
2 checks passed
@hohwille hohwille added this to the release:2024.02.001 milestone Jan 22, 2024
@hohwille hohwille added the story-review marks PRs that will be presented in the sprint-review label May 3, 2024
@hohwille hohwille added the reviewed Marks PRs that have been presented in the sprint-review meeting or that do not need to be presented. label Oct 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
reviewed Marks PRs that have been presented in the sprint-review meeting or that do not need to be presented. story-review marks PRs that will be presented in the sprint-review
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

Duplicate message if tool is already installed
4 participants